Skip to content

Conversation

@mar-cf
Copy link
Contributor

@mar-cf mar-cf commented Oct 7, 2025

No description provided.

@jmorrell-cloudflare
Copy link
Contributor

jmorrell-cloudflare commented Oct 7, 2025

I don't think multiple checksums are allowed: https://developers.cloudflare.com/r2/api/workers/workers-api-reference/#r2putoptions

Note
Only a single hashing algorithm can be specified at once.

That said, this would be better if we ever relaxed that requirement in the future. However I think it's harder to query, so I'm a little torn.

@mar-cf
Copy link
Contributor Author

mar-cf commented Oct 10, 2025

Alternatively, we could just set the first seen checked in a preset order.

And/or post under a different tag, checksum2 or something along those lines.

Only a single hashing algorithm can be specified at once.

That may be true, but I noticed that doesn't hold all the time.

@jmorrell-cloudflare LMK what you prefer

Copy link
Contributor

@jmorrell-cloudflare jmorrell-cloudflare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still want to land this go for it

@mar-cf mar-cf marked this pull request as ready for review October 29, 2025 17:38
@mar-cf mar-cf requested review from a team as code owners October 29, 2025 17:38
@mar-cf mar-cf force-pushed the mar/stw-r2-checksum-dup branch from 70ba844 to 58a1f15 Compare October 30, 2025 00:05
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 30, 2025

CodSpeed Performance Report

Merging #5252 will not alter performance

Comparing mar/stw-r2-checksum-dup (3c3e8b6) with main (8e90d6d)

Summary

✅ 34 untouched
⏩ 9 skipped1

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@mar-cf
Copy link
Contributor Author

mar-cf commented Oct 30, 2025

@fhanau confirmed with R2 that it's standard to have multiple checksums, so it's something we should support.

For ease of use, we can still designate one (picked in set order) set under some generalized checksum tag, in addition to this.

@jmorrell-cloudflare
Copy link
Contributor

If multiple checksums are allowed, then I'm fine merging this as-is

@mar-cf mar-cf enabled auto-merge October 31, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants